Python: Promote XXE and XML-bomb queries #8634

RasmusWL · 2022-03-31T16:57:10Z

This was a draft PR to ensure we don't merge it before we have agreed with JS/Ruby about the new XmlParsing concept, but they have said good for it, so we're ready to go 👍

With this PR, I have copied the query setup from JS (Ruby only has XXE query). This means that no query will alert on 'DTD retrieval', which I will postpone for future work (this PR is already quite big).

After internal discussion, these will replace the `XmlEntityInjection` query, so we can have separate severities on DoS and the other (more serious) attacks. Note: These clearly don't work, since they are verbatim copies of the JS code, but I split it into multiple commits to clearly highlight what changes were made.

I changed a few QLdocs so they fit the style we have used in Python... although I surely do regret having introduced a new style for how these QLDocs look :D

Note that most of the testing happens in the framework specific tests, with an inline-expectation test

Which doesn't raise that syntax error (at least not on my laptop)

Kept the test of SimpleXmlRpcServer, and kept the qhelp so it can be used to write the new qhelp files

and remove the old copy, we don't need it anymore :)

I found this resource quite good myself at least :)

Since there are other XML vulnerabilities that are not about parsing, this is more correct.

I forgot about the existing ones when I promoted it

I didn't find a good way to actually share the stuff, so we kinda just have 2 things that look very similar :|

And it's not possible to provide a parser argument either

- `XMLEtree` to `XmlEtree` - `XMLSax` to `XmlSax` - `LXML` to `Lxml` - `XMLParser` to `XmlParser`

This also means that the detection of the values passed to these keyword arguments will no longer just be from a local scope, but can also be across function boundaries.

RasmusWL · 2022-04-06T13:51:55Z

I tried to see whether API::CallNode could be used to merge the xml.etree.ElementTree modeling, but I couldn't find a nice way to do this, so I left them in this slightly ugly state where they sort of share logic, but doesn't actually share the code.

RasmusWL · 2022-04-22T08:45:59Z

Performance evaluation looks good 👍

@erik-krogh

Originally made by @erik-krogh in https://github.com/github/codeql/pull/8693/files#diff-9627c1fb9a1cc77fb93e6b7e31af1a4fa908f2a60362cfb34377d24debb97398 Could not be applied directly to this PR, since this PR deletes the file.

yoff

Good thorough job. Only minor comments. It looks like you deleted a poc (this_directory_not_extracted)?

python/ql/src/Security/CWE-776/XmlBomb.qhelp

python/ql/lib/change-notes/2022-03-29-add-taint-for-StringIO.md

yoff · 2022-05-05T14:26:09Z

python/ql/lib/semmle/python/frameworks/Lxml.qll

+   */
+  private class FileAccessFromLxmlParsing extends LxmlParsing, FileSystemAccess::Range {
+    FileAccessFromLxmlParsing() {
+      this = API::moduleImport("lxml").getMember("etree").getMember(["parse", "parseid"]).getACall()


I wondered if a utility to get the function name could make this smoother (like MethodCallNode has).

RasmusWL · 2022-05-09T08:49:14Z

It looks like you deleted a poc (this_directory_not_extracted)?

Since the old PoC was located with the framework testing code, and the framework testing code was moved to multiple folders, I decided to create a new top-level directory to contain that code: https://github.com/github/codeql/blob/714465bf39d97e31aa6f0a7aa01c57e16f3c3078/python/PoCs/XmlParsing/PoC.py -- I guess I should have mentioned this in a comment 😳 I'm not sure it's the best solution ever, since this means that the framework testing code is far removed from this PoC, but I also couldn't come up with a better solution 🤔

Co-authored-by: yoff <lerchedahl@gmail.com>

github-advanced-security

Found 5 vulnerabilities.

python/ql/lib/semmle/python/frameworks/Stdlib.qll

+    private DataFlow::TypeTrackingNode instance(DataFlow::TypeTracker t) {
+      t.start() and
+      result instanceof InstanceSource
+      or
+      exists(DataFlow::TypeTracker t2 | result = instance(t2).track(t2, t))
+    }


python/ql/lib/semmle/python/frameworks/Stdlib.qll

+    }
+
+    /** Gets a reference to an instance of `io.StringIO`/`io.BytesIO`. */
+    DataFlow::Node instance() { instance(DataFlow::TypeTracker::end()).flowsTo(result) }


python/ql/test/experimental/meta/ConceptsTest.qll

python/ql/lib/semmle/python/frameworks/Stdlib.qll

RasmusWL · 2022-05-09T09:04:24Z

Should have fixed the QL4QL alerts now as well 👍

yoff

I do not want to drag this out over little refactoring, but since you now did the one for Lxml (that I recall considering), I want to suggest the only other one, I considered. (I think the two branches of elementTreeInstance are fine to keep apart, since they have different semantics).

python/ql/lib/semmle/python/frameworks/Stdlib.qll

Co-authored-by: yoff <lerchedahl@gmail.com>

yoff

LGTM

RasmusWL and others added 30 commits March 31, 2022 09:52

Python: Adjust Xxe/XmlBomb for Python

e45f9d6

I changed a few QLdocs so they fit the style we have used in Python... although I surely do regret having introduced a new style for how these QLDocs look :D

Python: Add simple test of Xxe/XmlBomb

91795b8

Note that most of the testing happens in the framework specific tests, with an inline-expectation test

Python: Adjust XXE PoC for newer lxml versions

a1d88e3

Which doesn't raise that syntax error (at least not on my laptop)

Python: XXE: Add example of exfiltrating data through dtd-retrival

57b9780

Python: Add taint for StringIO and BytesIO

769f569

Python: Delete XmlEntityInjection.ql

c365337

Kept the test of SimpleXmlRpcServer, and kept the qhelp so it can be used to write the new qhelp files

Python: Adjust XXE qhelp

b00766b

and remove the old copy, we don't need it anymore :)

Python: Adjust XmlBomb.qhelp from JS

56b9c89

Python: Add PortSwigger link to Xxe.qhelp

9caf4be

I found this resource quite good myself at least :)

Python: Promote XMLParsing concept

e005a5c

Python: => XMLParsingVulnerabilityKind

e45288e

Since there are other XML vulnerabilities that are not about parsing, this is more correct.

Python: Promote XMLParsing concept test

35ccba2

Python: Make XMLParsing a Decoding subclass

1ea4bcc

Python: Rename lxml XPath tests

c4473c5

Python: Handle XMLParser().close() for XPath

3040adf

Python: Promote lxml parsing modeling

80b5cde

Python: Promote xmltodict modeling

7f5f767

Python: Promote xml.etree modeling

64aa503

Python: Add some links in QLDocs

a315aa8

Python: Add note about parseid/XMLID

6774085

Python: Model lxml.etree.XMLID

12cbdcd

Python: Model lxml.iterparse

386ff53

Python: Model file access from XML parsing

543454e

Python: Add test showing misalignment of xml.etree modeling

db43d04

Python: Merge xml.etree.ElementTree models

70b3eec

I forgot about the existing ones when I promoted it

Python: Align xml.etree.ElementTree modeling

05bb0ef

I didn't find a good way to actually share the stuff, so we kinda just have 2 things that look very similar :|

Python: Promote xml.sax and xml.dom.* modeling

e112697

Python: xml.sax.parse is not a method call

1d7cec6

And it's not possible to provide a parser argument either

Python: Extend FileSystemAccess for xml.sax and xml.dom.* parsing

b4c0065

RasmusWL mentioned this pull request Apr 5, 2022

Python: Rewrite concepts to use extends ... instanceof ... #8668

Merged

RasmusWL added 3 commits April 6, 2022 12:56

Merge branch 'main' into promote-xxe

23637fd

Python: Rename more XML classes to follow convention

c784f15

- `XMLEtree` to `XmlEtree` - `XMLSax` to `XmlSax` - `LXML` to `Lxml` - `XMLParser` to `XmlParser`

Python: Use new API::CallNode for XML constant check

f2f0873

This also means that the detection of the values passed to these keyword arguments will no longer just be from a local scope, but can also be across function boundaries.

Python: Change XmlBomb vulnerability kind

7728b6c

RasmusWL requested a review from a team April 7, 2022 13:30

RasmusWL added 4 commits April 7, 2022 15:37

Python: Rename sink definitions for XXE/XML bomb

405480c

Python: Move last XXE/XML bomb out of experimental

8191be9

Python: Fix SimpleXmlRpcServer.expected

517444b

Merge branch 'main' into promote-xxe

bb6969a

RasmusWL marked this pull request as ready for review April 20, 2022 11:48

RasmusWL added 2 commits May 2, 2022 11:25

Merge branch 'main' into promote-xxe

5f01fc2

Python: Refactor SaxParserSetFeatureCall

714465b

Originally made by @erik-krogh in https://github.com/github/codeql/pull/8693/files#diff-9627c1fb9a1cc77fb93e6b7e31af1a4fa908f2a60362cfb34377d24debb97398 Could not be applied directly to this PR, since this PR deletes the file.

yoff requested changes May 5, 2022

View reviewed changes

RasmusWL and others added 2 commits May 9, 2022 10:53

Python: Apply suggestions from code review

f5854f3

Co-authored-by: yoff <lerchedahl@gmail.com>

Python: Slight refactor of LxmlParsing

f22bd03

github-advanced-security bot found potential problems May 9, 2022

View reviewed changes

RasmusWL added 2 commits May 9, 2022 11:00

Python: Fix casing of XMLDomParsing

3634922

Python: Fix singleton set

de05b10

RasmusWL requested a review from yoff May 9, 2022 09:52

yoff requested changes May 9, 2022

View reviewed changes

python/ql/lib/semmle/python/frameworks/Stdlib.qll Show resolved Hide resolved

Python: Apply suggestions from code review

4a67891

Co-authored-by: yoff <lerchedahl@gmail.com>

RasmusWL requested a review from yoff May 9, 2022 14:37

yoff approved these changes May 9, 2022

View reviewed changes

yoff merged commit b6605bc into github:main May 9, 2022

RasmusWL deleted the promote-xxe branch May 10, 2022 08:46

Python: Promote XXE and XML-bomb queries #8634

Python: Promote XXE and XML-bomb queries #8634

Uh oh!

Conversation

RasmusWL commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RasmusWL commented Apr 6, 2022

Uh oh!

RasmusWL commented Apr 22, 2022

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yoff May 5, 2022

Choose a reason for hiding this comment

Uh oh!

RasmusWL commented May 9, 2022

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Check warning

Check warning

Uh oh!

Uh oh!

RasmusWL commented May 9, 2022

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

RasmusWL commented Mar 31, 2022 •

edited

Loading